refactor: its a keychain #6
Conversation
|
@diasdavid A review and then NPM release would be great. Need to include the package in |
| @@ -5,12 +5,12 @@ const forge = require('node-forge') | |||
| const util = require('./util') | |||
|
|
|||
| class CMS { | |||
There was a problem hiding this comment.
PKCS #7: Cryptographic Message Syntax; [rfc 2315](https://tools.ietf.org/html/rfc2315]
There was a problem hiding this comment.
Thanks for clarifying. Mind adding it as a comment above the class?
| } | ||
|
|
||
| this.keystore = keystore | ||
| this.keychain = keychain | ||
| } | ||
|
|
||
| createAnonymousEncryptedData (name, plain, callback) { |
There was a problem hiding this comment.
What is the definition of AnonymousEncryptedData? Why does it need to exist?
There was a problem hiding this comment.
Will add documentation, real soon. This creates cryptographically secure data. That is data that can only be read by the holder of the key.
There was a problem hiding this comment.
There was a problem hiding this comment.
How is that different from regular encrypted data?
There was a problem hiding this comment.
encrypted data is just a blob. CMS provides structure. It particular it describes the data encryption algorithm and parameters. Also it contains a recipientInfo that describes the key that can be used to obtain the data encryption key.
There was a problem hiding this comment.
There was a problem hiding this comment.
I see, so this function name is more than just describing what it is it actually points to a data structure defined by CMS/PKCS#7. Please make sure to add this as a comment so that others don't get confused. (I'm always concerned on having design info on people's heads)
There was a problem hiding this comment.
I have written design notes in richardschneider/ipfs-encryption please start with richardschneider/ipfs-encryption#3 and richardschneider/ipfs-encryption#2. Then follow the hyper-links.
| if (err) { | ||
| return callback(err) | ||
| } | ||
|
|
||
| try { | ||
| const privateKey = forge.pki.decryptRsaPrivateKey(key, self.keystore._()) | ||
| const privateKey = forge.pki.decryptRsaPrivateKey(key, self.keychain._()) |
There was a problem hiding this comment.
Please avoid node-forge. We used it before and it is a HUGE dependency. Also last time I checked it was using UMD instead of CJS or ES6 modules which makes it a PITA to import.
There was a problem hiding this comment.
Still on #7, the crypto primitives should come from the libp2p-crypto. Let's then figure out if we can just extract these functions out of node-forge or another package on npm.
There was a problem hiding this comment.
Enhancing libp2p-crypto will take time. It needs to
- create/read PKCS 8 (Key exchange js-libp2p-crypto#114)
- create/read PKCS 7 CMS
- support PBKDF2
- support various hash and HMAC algorithms
- support OID
- support ASN.1
- ...
Crypto is a big issue.
My game plan is to initially use another package (node.forge) so that ipfs key and ipfs crypto can be implemented. Then we can gradually enhance libp2p2-crypto.
There was a problem hiding this comment.
My game plan is to initially use another package (node.forge) so that ipfs key and ipfs crypto can be implemented. Then we can gradually enhance libp2p2-crypto.
Yes, we can use another package. Note, libp2p-crypto is just a bundle of other modules, we do not implement any crypto functions ourselves. However, for questions of consistency, we want libp2p crypto to expose the crypto primitives.
Can you add these funcs you need there?
| @@ -5,12 +5,12 @@ const forge = require('node-forge') | |||
| const util = require('./util') | |||
|
|
|||
| class CMS { | |||
There was a problem hiding this comment.
Thanks for clarifying. Mind adding it as a comment above the class?
| throw new Error('keystore is required') | ||
| constructor (keychain) { | ||
| if (!keychain) { | ||
| throw new Error('keychain is required') | ||
| } |
There was a problem hiding this comment.
You could use assert(keychain, 'keychain is required') to make this a one liner, but a if is also fine.
There was a problem hiding this comment.
True, will do.
| } | ||
|
|
||
| this.keystore = keystore | ||
| this.keychain = keychain | ||
| } | ||
|
|
||
| createAnonymousEncryptedData (name, plain, callback) { |
There was a problem hiding this comment.
I see, so this function name is more than just describing what it is it actually points to a data structure defined by CMS/PKCS#7. Please make sure to add this as a comment so that others don't get confused. (I'm always concerned on having design info on people's heads)
| if (err) { | ||
| return callback(err) | ||
| } | ||
|
|
||
| try { | ||
| const privateKey = forge.pki.decryptRsaPrivateKey(key, self.keystore._()) | ||
| const privateKey = forge.pki.decryptRsaPrivateKey(key, self.keychain._()) |
There was a problem hiding this comment.
Still on #7, the crypto primitives should come from the libp2p-crypto. Let's then figure out if we can just extract these functions out of node-forge or another package on npm.
|
@richardschneider is this PR still needed? |
|
@diasdavid No its not required. |
Just an internal name change